Improve self-testing capabilities #2119
Conversation
`expect_success()` and `expect_failure()` are now stricter, testing the full set of assumptions (i.e. exactly one success/failure and zero failures/successes). This means that `expect_no_successes()` and `expect_no_failures()` can be deprecated. Fixes #2110
| #' @param message Check that the failure message matches this regexp. | ||
| #' @param ... Other arguments passed on to [expect_match()]. | ||
| #' @export | ||
| expect_success <- function(expr) { |
There was a problem hiding this comment.
This seems likely to be a breaking change. I guess you have a system for revdep checking before release, but it might be worth trying out on a sample of 50/100 packages now to see how big the impact might be.
There was a problem hiding this comment.
Looking at https://github.com/search?q=org%3Acran+expect_success+path%3A%2F%5Etests%5C%2Ftestthat%5C%2F%2F&type=code suggests that there will be a few packages that this breaks, but most of the uses of expect_success() look incorrect and think PRs that just remove expect_success() shouldn't be too hard for me to do.
| "Creating reference" | ||
| ) | ||
| ) | ||
| expect_known_output(cat("ok!\n"), file) |> |
There was a problem hiding this comment.
the 4.1.0 requirement is so satisfying 🧘♂️
There was a problem hiding this comment.
The simple joys of being able to use R features that were released 5 years ago 😆
|
|
||
| test_that("succeed always succeeds", { | ||
| expect_success(succeed()) | ||
| expect_failure(expect_failure({})) |
There was a problem hiding this comment.
i wonder if it might improve readability in this file in particular to alias expect_failure, something like meta_expect_failure = expect_failure, then any given test will look more like "we're testing the behavior of expect_failure" more clearly by not repeating the same name.
There was a problem hiding this comment.
I worry that would imply there's an independent implementation rather than using the expectation to test itself (which does seem dangerous, but I can't see a better way currently).
There was a problem hiding this comment.
to clarify I would define the alias at the top of the file, or perhaps even inside each test_that(), so that it's perfectly clear it's just a copy.
There was a problem hiding this comment.
I'll continue to contemplate that as I iterate, but I'm going to leave it off this PR for now.
| expect_success(succeed()) | ||
| expect_failure(expect_failure({})) | ||
| expect_failure(expect_failure(succeed())) | ||
| expect_failure(expect_failure({ |
There was a problem hiding this comment.
a bit torn but worth considering adding the parallel test with two fail()s, even though the second can't be reached?
(edit: I see it's done below in direct tests of capture_success_failure(), my taste is to channel through user-facing APIs where possible)
There was a problem hiding this comment.
Agreed, I think that's a good place to reinforce that we should only ever see one fail
|
|
||
| ## Testing your expectations | ||
|
|
||
| testthat comes with three expectations designed specifically to test expectations: `expect_success()` and `expect_failure()`: |
There was a problem hiding this comment.
To me this is all downstream of the principle you conveyed in #1469 -- custom expectations should only give one pass/fail condition. I'm not sure that's conveyed here.
A simple example like what I'd tried in #1469 might be illustrative: two versions of a custom expectation which superficially look equivalent, but the bad one uses multiple succeed() and the preferred one uses if (!...) fail() instead.
There was a problem hiding this comment.
Yeah, agreed. My plan is to discuss that more in #2125
| last_failure <- NULL | ||
|
|
||
| withRestarts( | ||
| withCallingHandlers( | ||
| expr, | ||
| expectation_failure = function(cnd) { | ||
| last_failure <<- cnd | ||
| n_failure <<- n_failure + 1 | ||
| # Finish the test without bubbling up | ||
| invokeRestart("failed") |
There was a problem hiding this comment.
IIUC n_failure is either 0 or 1, because we're unwinding back to failed at the first failure?
In this case I think it would be a little clearer to remove n_failure and rename last_failure (whose "last" prefix imply there might be more than one assigned here) to just failure. Then you test for !is.null(failure) instead of n_failure > 0.
Also fine as is but then I'd add a comment saying we expect at most 1 failure.
There was a problem hiding this comment.
I think this is a good suggestion but I'm going to leave it until after #2129 because there I am encountering a weird situation where fail() is not causing an expectation to exit and I think that this might help me debug it.
| } else if (status$n_failure > 1) { | ||
| # This should be impossible, but including for completeness | ||
| fail("Expectation failed more than once") |
There was a problem hiding this comment.
oh yeah then you wouldn't need this branch
Co-authored-by: Lionel Henry <[email protected]>
expect_success()andexpect_failure()are now stricter, testing the full set of assumptions (i.e. exactly one success/failure and zero failures/successes). This means thatexpect_no_successes()andexpect_no_failures()can be deprecated.Fixes #2110
@lionel- could you please take a look at my restarts and reassure yourself that I'm using them correctly?
@MichaelChirico I'd love any thoughts on the documentation. It's going to take a few PRs to dig myself out of this hole, so no guarantees I'll make any bigger changes here, but I'll definitely consider them for future work.